Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding optional jsonp parameter to events/get_data #128

Merged
merged 1 commit into from
Feb 5, 2013

Conversation

gingerlime
Copy link
Contributor

  • using the same code / structure as render

* using the same code / structure as `render`
@gingerlime
Copy link
Contributor Author

I'm trying to use the events/get_data to retrieve event annotation data for giraffe graphite dashboard. It would be useful to be able to pull data using jsonp rather than just plain-json. This is my first graphite pull request, so hope I'm sticking to the coding standards of the project. I'm also not really sure if this pull request should go into 0.9.x or master branch (or both?).

@corywright
Copy link

I also would like to see jsonp available where ever json currently is supported.

@drawks
Copy link
Member

drawks commented Jan 23, 2013

I couldn't care less about inclusion or not of this feature, but would like to ensure that we take into account the difference in valid characters in the key side of the has between eval-able javascript (required by jsonp) and pure json. Who ever finally commits to implementing this would do good by the project to ensure special case handling for the few characters which are legal in json but aren't in javascript.

This link explains:
http://timelessrepo.com/json-isnt-a-javascript-subset

@gingerlime
Copy link
Contributor Author

First of all, thanks for the interesting link. I learned something new and quite interesting.

A couple of comments:

  • I'm not sure how this pull request makes the situation any different. Perhaps it uses the same implementation as where jsonp is used in render, which means if later this needs to be fixed it would need to be fixed in two places?
  • I just did a quick test using the text with the whitespace from the link, and then using the standard json module as well as simplejson (either of those as far as I see are used by graphite in utils). Perhaps I'm wrong, but it looks to me like both these modules handle the conversion correctly already:
Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> s = '{"JSON":"ro
cks!"}'
>>> s
'{"JSON":"ro\xe2\x80\xa8cks!"}'
>>> import json
>>> json.dumps(s)
'"{\\"JSON\\":\\"ro\\u2028cks!\\"}"'
>>> import simplejson
>>> simplejson.dumps(s)
'"{\\"JSON\\":\\"ro\\u2028cks!\\"}"'

mleinart added a commit that referenced this pull request Feb 5, 2013
adding optional jsonp parameter to events/get_data
@mleinart mleinart merged commit 7642734 into graphite-project:0.9.x Feb 5, 2013
mleinart added a commit that referenced this pull request Feb 5, 2013
adding optional jsonp parameter to events/get_data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants